Skip to content

fix: IPC reactor delivered stale responses to recycled client slots and died of SIGPIPE - #25150

Closed
charlielye wants to merge 3 commits into
nextfrom
cl/ipc-reactor-disconnect-cleanup
Closed

fix: IPC reactor delivered stale responses to recycled client slots and died of SIGPIPE#25150
charlielye wants to merge 3 commits into
nextfrom
cl/ipc-reactor-disconnect-cleanup

Conversation

@charlielye

@charlielye charlielye commented Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

The bug

ipc::IpcServer::run_reactor() keeps per-connection response-ordering state (arrival-order sequence counter + reorder stash) keyed by the transport's client slot id. SocketServer recycles slot ids (find_free_slot() returns the lowest freed slot) and nothing cleared the reactor's state when a connection ended. A connection dying with requests still in flight — routine in production: bb-avm-sim processes are killed on cancellation and teardown while their wsdb requests are outstanding — leaves "zombie" sequence entries on its slot, with two timing-dependent failure modes:

  1. Response misdelivery. If a new connection is accepted onto the freed slot before the zombie completes, the new connection's first response is stashed behind the zombie's sequence number, and the zombie's late completion is released to the new connection as its first frame. TS clients correlate responses positionally (no request-id envelope — the reactor's FIFO release is the correctness contract), so a single leaked frame shifts every subsequent response onto the wrong caller: wrong-type decodes or right-type values for the wrong parameters.
  2. Whole-server SIGPIPE death. If the zombie completes while the slot's fd is closed but not yet recycled, the reactor writes the response to the dead peer. send() passed no MSG_NOSIGNAL and nothing ignored SIGPIPE, so the write killed the entire server process — taking down every other client, including ones that never disconnected. Deterministically reproducible: any client that pipelines reads over UDS to aztec-wsdb and destroys its connection mid-flight killed the server within milliseconds (aztec-wsdb exited unexpectedly (code=null, signal=SIGPIPE)).

The fix

The slot table was an SHM-ism that had leaked into the socket transport: MPSC-SHM ids are physical ring indices and must recycle, but nothing about UDS wants recycled ids — idiomatic socket servers tie connection state to an identity that dies with the connection. So rather than guarding the recycled-id hazard, the socket transport now removes it:

  • SocketServer client ids are monotonic and never reused (next_client_id_++; id-keyed maps replace the dense slot vector, find_free_slot() is gone). A connection's identity cannot be inherited by a later connection, so a late respond() targets an id that no longer exists anywhere.
  • IpcServer::drain_disconnected_clients() (new hook): transports report client ids whose connection ended; the reactor erases their reorder state. With never-reused ids this is garbage collection, not a correctness-ordering mechanism — and respond() now find()s instead of creating, so a completion for an erased connection is dropped rather than written to a dead fd. (The hook's doc notes that a transport with recycled ids — SHM — must not adopt it as-is; that's the known SHM follow-up.)
  • MSG_NOSIGNAL on SocketServer::send() (Linux) and SO_NOSIGPIPE on accepted fds (macOS), plus SIGPIPE → SIG_IGN in install_default_signal_handlers(): a write that races a disconnect yields EPIPE (already handled — the send loop disconnects the client), never a process-killing signal.
  • UdsIpcClient retries ECONNRESET on connect (TS): under connection churn a connect can race the server's accept loop and get reset; previously only ECONNREFUSED/ENOENT/ETIMEDOUT/EAGAIN were retried, so a transient reset surfaced as a hard connect failure.

Tests

C++ (ipc-runtime, socket.test.cpp):

  • ReactorDropsStaleResponsesAndNeverReusesIds: the failure-mode-1 choreography (a connection dies with a gated response in flight; a new connection arrives; the zombie completes) — asserts the new connection receives exactly its own frame and a fresh client id. The recycled-slot version of this scenario was RED 20/20 before the fix (first frame carried the dead connection's payload + an extra leaked frame); GREEN now, 10× repeats.
  • ReactorSequentialConnectionsAreIndependent: clean-handover control, also pinning the never-reused-id invariant.
  • ReactorSurvivesResponseToDeadClient: reactor drops/fails responses to a dead client without dying. Note in-process writes to a just-closed peer can be absorbed by kernel buffering, so this alone cannot prove SIGPIPE immunity — hence the cross-process test below.

TS (yarn-project/world-state) — run against the rebuilt aztec-wsdb:

  • wsdb_sigpipe_death.test.ts: cross-process guard for failure mode 2. A long-lived monitor connection must keep reading correct answers through 20 rounds of an unrelated peer pipelining ~400 reads and destroying its connection mid-flight. Deterministically RED against the unfixed binary (server dead in ~12 ms, monitor gets read ECONNRESET); GREEN with the fix.
  • ipc_churn_correlation.test.ts: correlation load test where every response must prove it belongs to its own request by value, not just by type. Each connection plants a private fork with leaves derived from its own seed — connections share no observable state, so a cross-connection swap of same-type responses (invisible to the existing shared-state tests, where identical requests have identical answers) fails on wrong index/root/size. Legs: C sequential sanity; A single-connection pipelined reads + per-fork writes (reorder-stash pressure, no disconnects); D multi-connection read/write soak with no churn — readers assert recorded roots and exact leaf indices, writers pipeline append→read-after-write with exact-index asserts; B = D's workload plus a rotating mid-flight destroy + replace (slot recycling). WSDB_SOAK_MS extends D/B for grinding sessions (default ~4s for CI; 30s soak run clean).

Leg D is deliberately independent of this PR's fixes: it passes against the unfixed binary too (while Leg B fails there in ~56 ms), so it discriminates the disconnect-cleanup bug class from any other IPC/reorder/scheduler defect — a failure in D on any binary is a distinct bug.

Full ipc_runtime_tests suite passes (19/19); existing ipc_pipelined_read_correlation.test.ts passes against the rebuilt binary.

Context and follow-ups

Found while investigating a flaky noir-contracts TXE failure (Expected size in TreeStateReference deserialization). These fixes remove the only proven server-side sources of stale/mispaired frames. The flake's exact frame was never reproduced despite extensive adversarial testing (≈2B wire-exact decode-fuzz frames, hour-long identity-verified soaks, an ASAN matrix over wsdb/ipc-runtime/world_state, and ~4k grinds of the exact failing test) — so #25160 adds the observability to make any recurrence self-diagnosing: TXE server logs captured to their own CI log, and generated TS clients appending a hexdump of the offending frame to decode errors.

Remaining follow-ups, deliberately out of scope here:

  • The MPSC-SHM transport has the analogous stale-state gap (per-slot response rings are not reset when a client detaches and its physical ring slot is reclaimed). Slot claim there is client-driven, so it needs an occupancy/handshake guard rather than the socket transport's disconnect hook — see the warning on drain_disconnected_clients().
  • The TS clients' "response with no pending caller" path still only console.warns; upgrading it to fail loudly is a small separate change to the ipc-codegen templates.

@charlielye
charlielye force-pushed the cl/ipc-reactor-disconnect-cleanup branch 5 times, most recently from 240ee17 to 5cead56 Compare August 12, 2026 14:55
@charlielye
charlielye force-pushed the cl/ipc-reactor-disconnect-cleanup branch from 5cead56 to 0f052fe Compare August 12, 2026 15:06
@charlielye charlielye mentioned this pull request Aug 12, 2026
Merged
charlielye added a commit that referenced this pull request Aug 21, 2026
@charlielye
feat(ipc): request-id envelopes on every frame; responses in completi…
127dec3 
…on order (#25196)

Stacked on #25150 (contains its commit until it merges; review the head
commit only).

## What

Every ipc-runtime frame now carries a client-assigned request id, and
clients correlate responses by id instead of position:

```
[4-byte LE length][8-byte LE request id][payload]      (length counts id + payload)
```

The server echoes the id on the response and sends responses in
**completion order** — the reactor's reorder stash, its per-connection
sequence counters, and the `drain_disconnected_clients()` GC hook are
all deleted. `respond()` pushes onto a plain completion queue; the
reactor drains and sends. A slow request no longer delays a fast one's
response on the same connection (previously the stash re-serialized
release even though the wsdb scheduler completed reads concurrently).

Ids are per-connection, random-start (so a stale frame left in a
recycled MPSC-SHM ring slot by a previous occupant can never pair with a
live call — largely defusing the known SHM stale-ring follow-up), with 0
reserved for server-initiated frames.

## Why

The TXE flake investigation (#25150) established that positional
correlation makes any stale/mispaired frame a silent decode-roulette.
Ids make the whole mispairing class *structurally impossible* and
detectable: a frame that pairs with nothing is now a loud `failAll`
(“response for unknown request id …”) instead of a `console.warn` —
closing the original hardening ask.

## How (ipc-runtime only — codegen and all generated packages untouched)

- **C++ interface**: `IpcServer::receive(client, uint64_t& id)` /
`send(client, id, …)`; `run()` echoes internally. `IpcClient` gains
explicit-id `send`/`receive` virtuals plus **serial convenience
overloads** (`send(data,…)`/`receive(t)`) that auto-assign ids and
verify the echo — keeping the generated C++ clients and every FFI
binding **source-compatible**. Pipelined callers must use the
explicit-id API (the reactor tests do).
- **Mismatched-id semantics are per-transport**
(`IpcClient::may_have_stale_frames()`): SHM rings persist across
occupants, so a frame addressed to a previous occupant's id is an
*anticipated leftover* — the serial receive releases it and keeps
waiting for the real response, and the TS async client discards it with
a warning. Over UDS there is no reuse, so a foreign id is a genuine
desync and fails loudly (serial: close; TS: `failAll`).
- **Transports**: socket server/client (3-part writes, id strip on
read), SPSC + MPSC SHM rings (`ring_send_msg`/`ring_receive_msg` carry
the id inside the ring message), napi async glue (`call(id: bigint,
buf)`, callback delivers `(id, buffer)`).
- **TS**: `UdsIpcClient` and `NapiShmAsyncClient` hold `Map<bigint,
pending>` (random-start ids); `UdsIpcServer` parses/echoes.
`IpcClientAsync.call()` signature unchanged — consumers are untouched
(CdbIpcServer wraps `UdsIpcServer`, so it rides along).
- **C ABI**: `ipc_server_receive/send` gain the id parameter; the
handler-loop APIs (`ipc_server_run*`) and the serial client calls are
unchanged — so the **rust and zig bindings need zero changes**
(verified: they bind only those surfaces).

## Version-mismatch story

This is a wire-protocol break; binaries and TS packages must move in
lockstep (they already ship as matched pairs). Mismatches fail fast with
explicit messages, both directions: a frame shorter than the id field is
rejected as “IPC protocol mismatch (envelope ids); update the peer
binary/package” (client-side `failAll`, server-side disconnect + log).
Labs-side note: the acvm/wsdb npm + toolchain artifacts pick this up on
their next republish pinned to a commit containing this change; the rust
transport there is the same FFI crate, so no code change — just the
lockstep bump.

## Verification

- `ipc_runtime_tests` 21/21 release and **21/21 ASAN** — including
`SerialClientSkipsStaleFrameFromPreviousOccupant` (SHM: injected stale
frame is skipped, real response delivered) and
`SerialClientClosesOnForeignFrame` (UDS: same injection fails the call).
Pipelined tests converted from FIFO-order assertions to id-pairing
assertions (each request answered exactly once); the reactor tests now
also assert responses arrive **out of send order** under reversed-sleep
handlers — pinning the head-of-line-blocking removal — and the MPSC test
keeps its lost-wake stall probe.
- New TS tests: UDS unknown id → loud failAll; id-less (old-protocol)
frame → explicit protocol-mismatch error; SHM async client discards a
stale frame while resolving live calls (mock-addon unit tests, incl.
out-of-order pairing). `ipc-runtime/ts` suite 18/18.
- echo_example (generated clients over the new runtime, codegen
untouched): package + reliability tests green over **uds and shm**,
including the SyncApi.
- Cross-stack: rebuilt `aztec-wsdb` + new TS client running the
world-state suites — sigpipe blast-radius test, churn legs A/B/C/D with
60s soaks (~1.2M id-paired calls, ~1.4k mid-flight-disconnect churn
rounds), and the existing pipelined-read correlation suite over **both
transports** — all green.
@charlielye

charlielye commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Went in as part of #25196 (comment)

@charlielye charlielye closed this Aug 21, 2026
fcarreiro pushed a commit to aztec-labs-eng/aztec-node that referenced this pull request Aug 28, 2026
@charlielye @fcarreiro
feat(ipc): request-id envelopes on every frame; responses in completi…
3771930 
…on order (AztecProtocol/aztec-packages#25196)

Stacked on AztecProtocol/aztec-packages#25150 (contains its commit until it merges; review the head
commit only).

## What

Every ipc-runtime frame now carries a client-assigned request id, and
clients correlate responses by id instead of position:

```
[4-byte LE length][8-byte LE request id][payload]      (length counts id + payload)
```

The server echoes the id on the response and sends responses in
**completion order** — the reactor's reorder stash, its per-connection
sequence counters, and the `drain_disconnected_clients()` GC hook are
all deleted. `respond()` pushes onto a plain completion queue; the
reactor drains and sends. A slow request no longer delays a fast one's
response on the same connection (previously the stash re-serialized
release even though the wsdb scheduler completed reads concurrently).

Ids are per-connection, random-start (so a stale frame left in a
recycled MPSC-SHM ring slot by a previous occupant can never pair with a
live call — largely defusing the known SHM stale-ring follow-up), with 0
reserved for server-initiated frames.

## Why

The TXE flake investigation (AztecProtocol/aztec-packages#25150) established that positional
correlation makes any stale/mispaired frame a silent decode-roulette.
Ids make the whole mispairing class *structurally impossible* and
detectable: a frame that pairs with nothing is now a loud `failAll`
(“response for unknown request id …”) instead of a `console.warn` —
closing the original hardening ask.

## How (ipc-runtime only — codegen and all generated packages untouched)

- **C++ interface**: `IpcServer::receive(client, uint64_t& id)` /
`send(client, id, …)`; `run()` echoes internally. `IpcClient` gains
explicit-id `send`/`receive` virtuals plus **serial convenience
overloads** (`send(data,…)`/`receive(t)`) that auto-assign ids and
verify the echo — keeping the generated C++ clients and every FFI
binding **source-compatible**. Pipelined callers must use the
explicit-id API (the reactor tests do).
- **Mismatched-id semantics are per-transport**
(`IpcClient::may_have_stale_frames()`): SHM rings persist across
occupants, so a frame addressed to a previous occupant's id is an
*anticipated leftover* — the serial receive releases it and keeps
waiting for the real response, and the TS async client discards it with
a warning. Over UDS there is no reuse, so a foreign id is a genuine
desync and fails loudly (serial: close; TS: `failAll`).
- **Transports**: socket server/client (3-part writes, id strip on
read), SPSC + MPSC SHM rings (`ring_send_msg`/`ring_receive_msg` carry
the id inside the ring message), napi async glue (`call(id: bigint,
buf)`, callback delivers `(id, buffer)`).
- **TS**: `UdsIpcClient` and `NapiShmAsyncClient` hold `Map<bigint,
pending>` (random-start ids); `UdsIpcServer` parses/echoes.
`IpcClientAsync.call()` signature unchanged — consumers are untouched
(CdbIpcServer wraps `UdsIpcServer`, so it rides along).
- **C ABI**: `ipc_server_receive/send` gain the id parameter; the
handler-loop APIs (`ipc_server_run*`) and the serial client calls are
unchanged — so the **rust and zig bindings need zero changes**
(verified: they bind only those surfaces).

## Version-mismatch story

This is a wire-protocol break; binaries and TS packages must move in
lockstep (they already ship as matched pairs). Mismatches fail fast with
explicit messages, both directions: a frame shorter than the id field is
rejected as “IPC protocol mismatch (envelope ids); update the peer
binary/package” (client-side `failAll`, server-side disconnect + log).
Labs-side note: the acvm/wsdb npm + toolchain artifacts pick this up on
their next republish pinned to a commit containing this change; the rust
transport there is the same FFI crate, so no code change — just the
lockstep bump.

## Verification

- `ipc_runtime_tests` 21/21 release and **21/21 ASAN** — including
`SerialClientSkipsStaleFrameFromPreviousOccupant` (SHM: injected stale
frame is skipped, real response delivered) and
`SerialClientClosesOnForeignFrame` (UDS: same injection fails the call).
Pipelined tests converted from FIFO-order assertions to id-pairing
assertions (each request answered exactly once); the reactor tests now
also assert responses arrive **out of send order** under reversed-sleep
handlers — pinning the head-of-line-blocking removal — and the MPSC test
keeps its lost-wake stall probe.
- New TS tests: UDS unknown id → loud failAll; id-less (old-protocol)
frame → explicit protocol-mismatch error; SHM async client discards a
stale frame while resolving live calls (mock-addon unit tests, incl.
out-of-order pairing). `ipc-runtime/ts` suite 18/18.
- echo_example (generated clients over the new runtime, codegen
untouched): package + reliability tests green over **uds and shm**,
including the SyncApi.
- Cross-stack: rebuilt `aztec-wsdb` + new TS client running the
world-state suites — sigpipe blast-radius test, churn legs A/B/C/D with
60s soaks (~1.2M id-paired calls, ~1.4k mid-flight-disconnect churn
rounds), and the existing pipelined-read correlation suite over **both
transports** — all green.

The patch carries only this repo's share of that change: the two world-state
IPC tests (`ipc_churn_correlation.test.ts`, `wsdb_sigpipe_death.test.ts`).
The ipc-runtime work itself is foundation-side and landed there already.

Carried in the foundation patch series as
`labs-patches/0004-feat-ipc-request-id-envelopes-on-every-frame-respons.patch`,
added by AztecProtocol/aztec-packages#25333, from AztecProtocol/aztec-packages#25196.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@charlielye